Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(plugin-nm): Avoid duplicate copies inside aliased dependencies. #4571

Merged
merged 6 commits into from
Oct 18, 2022

Conversation

larixer
Copy link
Member

@larixer larixer commented Jun 23, 2022

What's the problem this PR addresses?

The PR:
#3631
attempted to fix peer dependendents finding aliased packages. However, it really didn't fix the issue for node modules linker. Peer dependents still were finding a different copy of a package, plus duplicated package is problematic for disk footprint and performance considerations.

How did you fix it?

I have essentially rolled back the effects of the abovementioned PR and its side effect of creating duplicate copies of aliased dependencies. I have also added a check to prevent circular symbolic links creation by the nm linker, which is a related problem, reported multiple times:
#3256
#3409
#4232

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

arcanis
arcanis previously approved these changes Jun 24, 2022
// can't fulfill the peer dependency promise. For the NM linker we test that it at least
// fulfills the require promise (installing dragon-test-11-a both under the aliased and original name).
// This is only possible with the PnP and pnpm linkers.
// The node-modules linker can't fullfil these requirements for aliased packages,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it'd make sense to have a page in the documentation explaining the tradeoffs between the different install strategies (and why some things cannot be implemented) 🤔

// This is only possible with the PnP and pnpm linkers.
// The node-modules linker can't fullfil these requirements for aliased packages,
// without resorting to symlinks and a layout resembling pnpm for aliased dependencies,
// which will be too different from the classic node_modules layout for all the other dependencies.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The symlink layout you described on Discord would only affect aliased dependencies, right? Given that they are fairly rare, that nothing would change for regular dependencies, and that it doesn't have the cyclic symlink issue, I feel like it might be worth a try - but your call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It should work. However, it needs to be implemented carefully. Plus it might be worth resorting to symlinks only when the dependency is actually peer dependend upon or when self references support is enabled. So, if we making this carefully, it's not trivial, need 2-3 days to spend for implementation and tests... Should be done in a separate PR I think.

@larixer larixer force-pushed the larixer/nm-aliases-and-circular-symlinks branch from 02ef138 to 4cf48ff Compare June 24, 2022 09:56
@merceyz merceyz enabled auto-merge (squash) October 17, 2022 23:41
@merceyz merceyz merged commit 38f0134 into master Oct 18, 2022
@merceyz merceyz deleted the larixer/nm-aliases-and-circular-symlinks branch October 18, 2022 16:33
merceyz added a commit that referenced this pull request Nov 3, 2022
…4571)

* Avoids creation of circular symlinks and duplicates inside aliases

* Add a test to check that duplicate copies of aliased packages are not created

* Stricter ident comparison for hoisted packages

* chore: update changelog

Co-authored-by: merceyz <merceyz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants